Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed access modifier for file ids #324

Closed
wants to merge 1 commit into from
Closed

Conversation

walkeryan
Copy link

File ID's should be able to be set as they are in the python package.

File ID's should be able to be set as they are in the python package.
@walkeryan
Copy link
Author

Good morning @romainhuet , @trevorcreech , @PaulMcMillan , @schnerd . I've been using this code reliably for the past week or so. Can we get this merged in so I don't have to continue maintaining the project and can switch back to the Nuget package? <3

@joseharriaga
Copy link
Collaborator

Thank you for your contribution, @walkeryan ! Based on our current design guidelines for .NET, our collection properties do not have setters. Instead, they are guaranteed to be initialized, and users can add or remove items directly without having to new up the collection. For example:

CodeInterpreter = new() 
{ 
    FileIds = { myFileId1, myFIleId2 } 
}

If needed, users can replace the contents of the collection by clearing the collection first and then adding the new contents. For example:

toolResources.CodeInterpreter.Clear();
toolResources.CodeInterpreter.Add(myFile1);
toolResources.CodeInterpreter.Add(myFile2);

One big benefit of this approach is that users can safely use collection properties without having to check if the property is null each time.

If we were to make a change related to this, we would like to be able to apply it consistently across the entire library. As such, I will close this PR for now, but your contribution here is a very helpful data point for us as we continue evolving our current design guidelines and polishing the library. So once again thank you!

@walkeryan
Copy link
Author

Thanks for the clarification, not sure why, but I tried it this way and had to switch over to the python package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants